[Preview] MSAL and shared token cache support by cormacpayne · Pull Request #9982 · Azure/azure-powershell

cormacpayne · 2019-09-05T20:21:57Z

Description

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate ChangeLog.md file(s) has been updated:
- For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
- A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
- the markdown help files have been regenerated using the commands listed here

…emove this dependency)

…to msal-support

tools/RepoTasks/RepoTasks.RemoteWorker/RepoTasks.RemoteWorker.csproj

src/Accounts/Authentication/Authentication/CustomWebUi.cs

src/Accounts/Authentication/Authentication/DelegatingAuthenticator.cs

markcowl · 2019-09-13T08:47:15Z

src/Accounts/Authentication/Authentication/Parameters/ManagedServiceIdentityParameters.cs

+        public ManagedServiceIdentityParameters(
+            IAzureEnvironment environment,
+            IAzureTokenCache tokenCache,
+            string tenantId,


Seems like this should contain parameters to contain identity type (SystemAssigned, ClientOrObjectId, ResourceId and identity id (string). These are all properties of IAzureAccount, but seems like we could use them to populate the account here.

Also, MSISecret, and MSiEndpoint

src/Accounts/Authenticators/AccessTokenAuthenticator.cs

src/Accounts/Authenticators/InteractiveUserAuthenticator.cs

markcowl · 2019-09-13T09:12:31Z

src/Accounts/Authenticators/ManagedServiceIdentityAuthenticator.cs

+
+            if (account.IsPropertySet(AppServiceManagedIdentityFlag))
+            {
+                return new ManagedServiceAppServiceAccessToken(account, environment, GetFunctionsResourceId(resourceId, environment), tenant);


Would it be cleaner to have one authenticator for AppServiceMSI first in the chain, and the authenticator for Compute MSI after?

src/Accounts/Authenticators/ServicePrincipalAuthenticator.cs

src/Accounts/Authenticators/SilentAuthenticator.cs

src/Accounts/Authentication/Authentication/Clients/AuthenticationClientFactory.cs

markcowl · 2019-09-13T09:21:15Z

src/Accounts/Authentication/Factories/AuthenticationFactory.cs

        public const string AppServiceManagedIdentityFlag = "AppServiceManagedIdentityFlag";

-        public const string CommonAdTenant = "Common",
+        public const string CommonAdTenant = "organizations",


Seems like we should have this constant in one place, and use it evwerywhere. Also, same question as before about whether thsi works with MSA

src/Accounts/Authentication/Factories/AuthenticationFactory.cs

markcowl · 2019-09-13T09:25:44Z

src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs

                }

-                DefaultContextKey = profile.DefaultContextKey ?? "Default";
+                DefaultContextKey = profile.DefaultContextKey ?? (profile.Contexts.Any() ? null : "Default");


Setting the defautl key to null if there are contexts but none selected - when would this occur?

Needs validate: If logged in in VS (not PS), when first time runing PS command (not select-context), should be prompted with "please select a context"

src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs

markcowl · 2019-09-13T09:34:08Z

src/Accounts/Authentication.ResourceManager/AzureRmProfile.cs

+            var accounts = authenticationClientFactory.ListAccounts();
+            if (!accounts.Any())
+            {
+                if (!Contexts.Any(c => c.Key != "Default" && c.Value.Account.Type == AzureAccount.AccountType.User))


I think this method could use a bit of breaking up.

RemoveUserContexts()
RemoveUnmatchedUserContexts()
etc.

Also, as stated above, we need soem tracing here so we can tell why a context was removed if soemthign goes wrong.

src/Accounts/Accounts/Common/AzureContextModificationCmdlet.cs

src/Accounts/Authentication/Authentication.csproj

src/Accounts/Authentication/Authentication/ProtectedFileTokenCache.cs

…acquiring common token

src/Accounts/Accounts/Models/RMProfileClient.cs

src/Accounts/Authentication/Authentication/Clients/AuthenticationClientFactory.cs

isra-fel · 2019-10-13T04:36:19Z

/azp run

azure-pipelines · 2019-10-13T04:36:38Z

Azure Pipelines successfully started running 3 pipeline(s).

isra-fel · 2019-10-13T05:49:41Z

/azp run

azure-pipelines · 2019-10-13T05:50:02Z

Azure Pipelines successfully started running 3 pipeline(s).

isra-fel · 2019-10-14T09:18:19Z

src/Accounts/Authenticators/DesktopAuthenticatorBuilder.cs

-                return authenticator as IAuthenticatorBuilder;
-            });
+            AppendAuthenticator(() => { return new InteractiveUserAuthenticator(); });
+            var defaultBuilder = new DefaultAuthenticatorBuilder();


Wouldn't that be 2 InteractiveUserAuthenticators 🤔 ?

to support IAzureMsalTokenCache

isra-fel · 2019-10-17T07:19:10Z

/azp run

azure-pipelines · 2019-10-17T07:19:30Z

Azure Pipelines successfully started running 3 pipeline(s).

cormacpayne added 10 commits September 4, 2019 12:43

Common code package updates

5e1c70d

Changes to Authenticators

5c2f22e

Changes to lib folder assemblies

c95077b

Add classes from ClientRuntime Authentication library (allows us to r…

17a60ca

…emove this dependency)

Add new client factory classes

3522bc5

Changes to AuthenticationFactory (CoR pattern used)

a672d35

Changes to AzureRmProfile (context update logic)

32f5bd8

Other changes in Accounts project

094542a

Other changes in Authentication project

b1a4104

Miscellaneous changes in other projects

08f6886

cormacpayne added the needs-review label Sep 5, 2019

cormacpayne assigned markcowl Sep 5, 2019

markcowl assigned barnstee and erich-wang and unassigned barnstee Sep 10, 2019

Merge branch 'master' of https://github.com/Azure/azure-powershell in…

00b20f0

…to msal-support

markcowl requested changes Sep 13, 2019

View reviewed changes

src/Accounts/Authentication/Authentication/Clients/AuthenticationClientFactory.cs Show resolved Hide resolved

markcowl requested changes Sep 13, 2019

View reviewed changes

src/Accounts/Accounts/Common/AzureContextModificationCmdlet.cs Show resolved Hide resolved

src/Accounts/Accounts/Common/AzureContextModificationCmdlet.cs Outdated Show resolved Hide resolved

markcowl requested changes Sep 13, 2019

View reviewed changes

src/Accounts/Authentication/Authentication.csproj Show resolved Hide resolved

src/Accounts/Authentication/Authentication/ProtectedFileTokenCache.cs Show resolved Hide resolved

markcowl added needs-revision and removed needs-review labels Sep 13, 2019

markcowl assigned cormacpayne and unassigned markcowl Sep 13, 2019

cormacpayne added 3 commits September 16, 2019 09:16

Resolving a few comments from feedback

6478c36

Add missing assemblies to Az.Accounts.psd1

4985264

Fix issue with ADFS authentication across different authenticators

2a772b5

cormacpayne added 2 commits September 17, 2019 13:42

CancellationToken support in authenticators, update error logic when …

9e5ad5b

…acquiring common token

Add logging and tracing for MSAL calls

71e05c1

markcowl requested changes Sep 20, 2019

View reviewed changes

src/Accounts/Accounts/Models/RMProfileClient.cs Show resolved Hide resolved

src/Accounts/Authentication/Authentication/Clients/AuthenticationClientFactory.cs Show resolved Hide resolved

markcowl assigned markcowl and isra-fel and unassigned erich-wang Sep 26, 2019

isra-fel mentioned this pull request Oct 12, 2019

Move to MSAL #10259

Closed

10 tasks

Temporarily patch linux failure

d3f9aa9

isra-fel reviewed Oct 14, 2019

View reviewed changes

adopt latest common code

551fc76

to support IAzureMsalTokenCache

isra-fel force-pushed the msal-support branch from 6c69f84 to b941dca Compare October 17, 2019 06:55

make test pass

fbcc1dc

isra-fel force-pushed the msal-support branch from 44f6f2a to fbcc1dc Compare October 21, 2019 09:04

isra-fel added 3 commits October 22, 2019 23:21

typo

6cd831a

use latest version of msal and extensions

921b7cd

preload netfx msal extensions; not msal

9c9c7c8

isra-fel force-pushed the msal-support branch from a640cbf to 9c9c7c8 Compare November 4, 2019 15:09

isra-fel mentioned this pull request Dec 16, 2019

Support ADFS #10260

Closed

12 tasks

dingmeng-xue added this to the S164 (2020-01-28) milestone Dec 30, 2019

isra-fel mentioned this pull request Jan 7, 2020

Nullreferenceexception in az accounts#10292 #10303

Merged

8 tasks

isra-fel mentioned this pull request Jan 22, 2020

MSAL support #10844

Closed

8 tasks

isra-fel closed this Jan 22, 2020

Conversation

cormacpayne commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markcowl Sep 13, 2019 • edited by cormacpayne Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markcowl Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markcowl Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markcowl Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

markcowl Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

isra-fel Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markcowl Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isra-fel commented Oct 13, 2019

Uh oh!

azure-pipelines bot commented Oct 13, 2019

Uh oh!

isra-fel commented Oct 13, 2019

Uh oh!

azure-pipelines bot commented Oct 13, 2019

Uh oh!

isra-fel Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

isra-fel commented Oct 17, 2019

Uh oh!

azure-pipelines bot commented Oct 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cormacpayne commented Sep 5, 2019 •

edited

Loading

markcowl Sep 13, 2019 •

edited by cormacpayne

Loading